fix: added snackbar and small ui changes in lux meter.#2747
Merged
Conversation
Contributor
Reviewer's GuideIntroduces user feedback for light sensor errors via Snackbar, enhances sensor lifecycle and platform-specific error handling, and refines the lux meter chart’s UI styling and responsiveness. Sequence diagram for light sensor error handling and Snackbar feedbacksequenceDiagram
actor User
participant LuxMeterScreen
participant LuxMeterStateProvider
participant Snackbar
User->>LuxMeterScreen: Open screen
LuxMeterScreen->>LuxMeterStateProvider: initializeSensors(onError)
alt Platform not supported or sensor error
LuxMeterStateProvider-->>LuxMeterScreen: onError callback with error message
LuxMeterScreen->>Snackbar: Show error message
else Sensor available
LuxMeterStateProvider-->>LuxMeterScreen: Sensor data updates
LuxMeterScreen->>User: Display chart
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey @Yugesh-Kumar-S - I've reviewed your changes - here's some feedback:
- The direct import of
dart:ioinluxmeter_state_provider.dartwill break web builds—use conditional imports or platform-agnostic abstractions to avoid build errors on non-native targets. - There’s a lot of duplicated logic across
_handleUnsupportedPlatform,_handleSensorError, and_handleSensorNotAvailable—consider unifying these into a single streamlined error‐handling flow to reduce repetition. - The chart layout code repeats padding and reserved size calculations for different breakpoints—extract those values into helper constants or methods to improve readability and maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The direct import of `dart:io` in `luxmeter_state_provider.dart` will break web builds—use conditional imports or platform-agnostic abstractions to avoid build errors on non-native targets.
- There’s a lot of duplicated logic across `_handleUnsupportedPlatform`, `_handleSensorError`, and `_handleSensorNotAvailable`—consider unifying these into a single streamlined error‐handling flow to reduce repetition.
- The chart layout code repeats padding and reserved size calculations for different breakpoints—extract those values into helper constants or methods to improve readability and maintainability.
## Individual Comments
### Comment 1
<location> `lib/view/luxmeter_screen.dart:146` </location>
<code_context>
- timeAxisLabel,
+ final reservedSizeLeft = screenWidth < 400 ? 27.0 : 30.0;
+ final reservedSizeRight = screenWidth < 400 ? 27.0 : 30.0;
+ return Padding(
+ padding: const EdgeInsets.only(right: 20.0),
+ child: LineChart(
+ LineChartData(
+ backgroundColor: Colors.black,
</code_context>
<issue_to_address>
Adding right padding to the chart may cause misalignment with other UI elements.
Consider using a responsive value or matching the card's padding to ensure consistent alignment across different screen sizes.
Suggested implementation:
```
// Define a responsive horizontal padding, or use the card's padding value if available
final horizontalPadding = screenWidth < 400 ? 12.0 : 20.0;
return Padding(
padding: EdgeInsets.only(right: horizontalPadding),
```
If the card's padding is defined elsewhere (e.g., as a constant or theme value), you should reference that value instead of defining `horizontalPadding` here. Also, consider applying the same `horizontalPadding` to the left side if you want symmetrical padding for better alignment.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Build successful. APKs to test: https://github.com/fossasia/pslab-android/actions/runs/16050092680/artifacts/3457890285 |
Co-authored-by: Padmal <CloudyPadmal@users.noreply.github.com>
CloudyPadmal
approved these changes
Jun 21, 2025
AsCress
suggested changes
Jun 22, 2025
Contributor
|
@Yugesh-Kumar-S This is good to go. Please resolve conflicts here. |
Contributor
Author
Done. |
CloudyPadmal
approved these changes
Jul 1, 2025
Contributor
Author
|
@AsCress Please review and merge this PR . I need to create a PR for luxmeter configuration screen. |
AsCress
approved these changes
Jul 4, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2746
Changes
Screenshots / Recordings
Checklist:
strings.xml,dimens.xmlandcolors.xmlwithout hard coding any value.strings.xml,dimens.xmlorcolors.xml.Summary by Sourcery
Add snackbar notifications for light sensor errors, refactor sensor initialization and disposal, and adjust the lux meter chart’s UI styling and layout.
New Features:
Enhancements: